Skip to content

Remove clear memory#150

Merged
timokau merged 2 commits intokiudee:masterfrom
timokau:remove-clear-memory
Aug 21, 2020
Merged

Remove clear memory#150
timokau merged 2 commits intokiudee:masterfrom
timokau:remove-clear-memory

Conversation

@timokau
Copy link
Copy Markdown
Collaborator

@timokau timokau commented Aug 21, 2020 •
edited
Loading

Description

Removing clear_memory and hash_file to reduce complexity and get one step further towards stateless / scikit-learn compatible estimators.

Motivation and Context

See description.

How Has This Been Tested?

Tests and pre-commit.

Does this close/impact existing issues?

Fixes #148, step towards #94 / #116.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.

@timokau timokau mentioned this pull request Aug 21, 2020
Closed
@codecov
Copy link
Copy Markdown

codecov Bot commented Aug 21, 2020 •
edited
Loading

Codecov Report

Merging #150 into master will increase coverage by 0.47%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #150      +/-   ##
==========================================
+ Coverage   57.77%   58.25%   +0.47%     
==========================================
  Files         113      113              
  Lines        6960     6848     -112     
==========================================
- Hits         4021     3989      -32     
+ Misses       2939     2859      -80
Impacted Files Coverage Δ
csrank/choicefunction/cmpnet_choice.py 94.59% <ø> (+2.28%) ⬆️
csrank/choicefunction/fate_choice.py 95.12% <ø> (+4.21%) ⬆️
csrank/choicefunction/feta_choice.py 66.20% <ø> (+0.66%) ⬆️
csrank/choicefunction/ranknet_choice.py 94.73% <ø> (+4.49%) ⬆️
csrank/core/cmpnet_core.py 97.72% <ø> (+10.34%) ⬆️
csrank/core/fate_network.py 70.15% <ø> (+3.49%) ⬆️
csrank/core/feta_network.py 64.89% <ø> (+3.43%) ⬆️
csrank/core/ranknet_core.py 97.72% <ø> (+10.34%) ⬆️
csrank/discretechoice/cmpnet_discrete_choice.py 100.00% <ø> (+3.22%) ⬆️
csrank/discretechoice/fate_discrete_choice.py 100.00% <ø> (+5.88%) ⬆️
... and 8 more

kiudee
kiudee previously approved these changes Aug 21, 2020
View reviewed changes
Copy link
Copy Markdown
Owner

@kiudee kiudee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thank you.

I agree with this simplification. Should it turn out that we need to clear memory in between different fit calls, we should anyway check what the best practices are.

@timokau
Copy link
Copy Markdown
Collaborator Author

timokau commented Aug 21, 2020

Travis has succeeded but that information apparently somehow got lost in transit. Can we restart that check?

@kiudee
Copy link
Copy Markdown
Owner

kiudee commented Aug 21, 2020

Restarting the build somehow did not fix it. I think the only thing we could do is to push something to trigger the whole process again. If that does not work I will investigate if there is some connection issue between Github and Travis CI (so far there is no error message).

timokau added 2 commits August 21, 2020 19:31
@timokau
Remove the clear memory function
8a44426 
Its use is no longer clear
(kiudee#148) and it introduces
unnecessary complications and additional state. Its removal also allows
us to remove the hash_file parameter.
@timokau
Remove hash_file parameter and attribute
76a9854 
Reduces complexity and brings us one step closer to removing state
(attributes) of estimators that is not set through the init parameters.
@timokau timokau force-pushed the remove-clear-memory branch from 1e165d0 to 76a9854 Compare August 21, 2020 17:31
@kiudee
Copy link
Copy Markdown
Owner

kiudee commented Aug 21, 2020 •
edited
Loading

Looks like this issue:
https://travis-ci.community/t/known-issue-travis-ci-reports-expected-waiting-for-status-to-be-reported-on-the-github-status-api-but-the-status-never-arrives/1154
I deactivated the status check for now until I update travis to the new format.

@timokau
Copy link
Copy Markdown
Collaborator Author

timokau commented Aug 21, 2020

Yeah I saw that too, very weird that it is just one report (all the previous ones seem outdated) and it isn't acknowledged yet.

Anyway, thanks for the review!

@timokau timokau merged commit 4e535f0 into kiudee:master Aug 21, 2020
@timokau timokau deleted the remove-clear-memory branch August 21, 2020 18:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kiudee kiudee kiudee approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

clear_memory still needed

2 participants

@timokau @kiudee